Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.69%. Comparing base (0a2d858) to head (d71743f).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##           master    #4476      +/-   ##
==========================================
- Coverage   80.70%   80.69%   -0.02%     
==========================================
  Files         156      156              
  Lines       16475    16474       -1     
  Branches     2799     2799              
==========================================
- Hits        13296    13293       -3     
- Misses       2296     2298       +2     
  Partials      883      883              

Q: Was this an actual problem or is this more of a nice-to-have small refactor? As I understand from the other PR, we're actively avoiding having more than one SentryLangchainCallback around at a time.

@sentrivana Yes, so the problem occurred when attempting to reproduce #4443. I was confused why, despite having two callbacks, the data in Sentry was not duplicated. I assumed that perhaps the DedupeIntegration was dropping stuff, but when I enabled debug=True in the SDK init, I got the following error, instead of a notice that the DedupeIntegration dropped the extra span:

 [sentry] ERROR: Internal error in sentry_sdk
Traceback (most recent call last):
  File "/Users/dszoke/Development/SDK/sentry-python/sentry_sdk/integrations/langchain.py", line 267, in on_llm_end
    span_data = self.span_map[run_id]
                ~~~~~~~~~~~~~^^^^^^^^
KeyError: UUID('2a2c72c8-66f1-4607-b4d2-bd6e93125c39')

The reason, of course, is that the first callback which ends deletes the span from the span_map, so that when the duplicate callback is called, that span is gone.

Of course, you are right that we try to only have one callback per LLM request in #4485. But, what happens if we have concurrent requests to LLMs, or some other more complex setup, where having two callbacks would be expected? I am unsure how realistic such setups are, but if they do occur, I am pretty confident that we don't want the span map to be shared between the instances.

Not against the change itself, seems reasonable in any case.

I was more pondering if this is behavior that needs testing (rather than a small internal refactor), but if you believe it is, I'd say test_span_map_is_instance_variable is sufficient. I'd remove test_span_map_not_class_attribute as it doesn't add any value -- we should test behavior, not implementation details, and the behavior in this case is covered by the first test already.

sounds reasonable, the first test is definitely more important anyhow